Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Regenerate language_pack #502

Closed
wants to merge 1 commit into from

Conversation

jonahgraham
Copy link

@jonahgraham jonahgraham commented Mar 8, 2023

This was done by running mvn package in _update.

The autogenerated files have one small error that was
previously fixed in jonahgraham@242dcf1
and this change therefore preserves that fix by excluding this
diff from the commit:

diff --git a/org.eclipse.tm4e.language_pack/javascript/Regular Expressions (JavaScript).tmLanguage b/org.eclipse.tm4e.language_pack/javascript/Regular Expressions (JavaScript).tmLanguage
index 54c7651c..1dda7806 100644
--- a/org.eclipse.tm4e.language_pack/javascript/Regular Expressions (JavaScript).tmLanguage
+++ b/org.eclipse.tm4e.language_pack/javascript/Regular Expressions (JavaScript).tmLanguage
@@ -230,7 +230,7 @@
                </dict>
        </dict>
        <key>scopeName</key>
-       <string>lngpck.source.js.regexp</string>
+       <string>source.js.regexp</string>
        <key>uuid</key>
        <string>AC8679DE-3AC7-4056-84F9-69A7ADC29DDD</string>
 </dict>

And bumping version numbers due to updated language packs

Fixes #501

@mickaelistria
Copy link
Contributor

@howlger What do you think?

@sebthom
Copy link
Member

sebthom commented Mar 8, 2023

In my personal extension repo I have a CI job that generates PRs automatically, maybe something like this could be implemented https://github.com/sebthom/extra-syntax-highlighting-eclipse-plugin/blob/main/.github/workflows/update-syntaxes.yml

@howlger
Copy link
Contributor

howlger commented Mar 8, 2023

+1, (based on the code review without testing): org.eclipse.tm4e.language_pack/javascript/Regular Expressions (JavaScript).tmLanguage is incorrect generated and overrides the manual fix 242dcf1. It has not been changed, so please exclude this file.

+1 also for @sebthom's idea

Independent from this, would it be better to have separate plugins and features, instead of a single plugin/feature containing everything?

This was done by running `mvn package` in `_update`.

The autogenerated files have one small error that was
previously fixed in 242dcf1
and this change therefore preserves that fix by excluding this
diff from the commit:

```diff
diff --git a/org.eclipse.tm4e.language_pack/javascript/Regular Expressions (JavaScript).tmLanguage b/org.eclipse.tm4e.language_pack/javascript/Regular Expressions (JavaScript).tmLanguage
index 54c7651c..1dda7806 100644
--- a/org.eclipse.tm4e.language_pack/javascript/Regular Expressions (JavaScript).tmLanguage
+++ b/org.eclipse.tm4e.language_pack/javascript/Regular Expressions (JavaScript).tmLanguage
@@ -230,7 +230,7 @@
                </dict>
        </dict>
        <key>scopeName</key>
-       <string>lngpck.source.js.regexp</string>
+       <string>source.js.regexp</string>
        <key>uuid</key>
        <string>AC8679DE-3AC7-4056-84F9-69A7ADC29DDD</string>
 </dict>
```

And bumping version numbers due to updated language packs

Fixes eclipse-tm4e#501
@jonahgraham
Copy link
Author

+1, (based on the code review without testing): org.eclipse.tm4e.language_pack/javascript/Regular Expressions (JavaScript).tmLanguage is incorrect generated and overrides the manual fix 242dcf1. It has not been changed, so please exclude this file.

I have updated the commit and the description of the PR. An updated look over it would be appreciated.

+1 also for @sebthom's idea

+1 From me too.

Independent from this, would it be better to have separate plugins and features, instead of a single plugin/feature containing everything?

If you mean having as many plug-ins as there are languages in the language pack, I'm not keen on that idea.

@howlger
Copy link
Contributor

howlger commented Mar 9, 2023

I have updated the commit and the description of the PR. An updated look over it would be appreciated.

Looks good to me. It would be good to test this by opening a simple example for each language/format so that the grammar is loaded. If I remember correctly, not all regular expressions work; lookbehind of an unknown number of characters is not supported and causes an error when loading the grammar. But that might have changed in the meantime. @sebthom, please correct me if this is not true. If you'd like me to test it rather than just roughly look it over, I can do that the week after next.

@sebthom
Copy link
Member

sebthom commented Mar 10, 2023

@howlger I'd appreciate if you could test the changes. it would actually also be nice to have an automated test that tries to load/parse the grammars.

@mickaelistria mickaelistria deleted the branch eclipse-tm4e:main March 13, 2023 08:29
@mickaelistria
Copy link
Contributor

TM4E is switching to main as main branch instead of master. Master branch will be removed soon. Can you please retarget this PR towards main branch?

@mickaelistria mickaelistria reopened this Mar 13, 2023
@mickaelistria mickaelistria changed the base branch from master to main March 13, 2023 09:51
@sebthom sebthom changed the title Regenerate language_pack chore: Regenerate language_pack Mar 13, 2023
@sebthom sebthom changed the title chore: Regenerate language_pack fix: Regenerate language_pack Mar 13, 2023
@github-actions github-actions bot added bug and removed bug labels Mar 13, 2023
@howlger
Copy link
Contributor

howlger commented Mar 31, 2023

+1

Sorry for the delay. I tested this pull request manually with sample files for the different formats.

I will try to fix the generation to avoid overwriting of the manual fix 242dcf1, and if I am successful, I will submit it as a separate pull request. After that, I also plan to look at how to automatically test it, to at least find the regular expressions that cause errors at runtime. But I can't promise anything and would be happy if someone else would do that.

@sebthom
Copy link
Member

sebthom commented Jun 11, 2023

Superseded by #550

@sebthom sebthom closed this Jun 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

language_pack is outdated?
4 participants